Skip to content

Conversation

@rghaddab
Copy link
Contributor

These are series of optimization for the new backend + some commits to syncronize ZMS to the upstream version


ret = settings_call_set_handler(arg->subtree, rc2, settings_zms_read_fn,
&read_fn_arg, (void *)arg);
return ret;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, you cannot return here. If subtree == "abc", the code is expected to return not only "abc" but also "abc/def" etc. You can only break if ret != 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Damian-Nordic you are right I will change that.
I actually didn't see any application using this feature of settings where you can load subtree "abc" and it will load abc/*

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. OpenThread uses it.

@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch 5 times, most recently from 44521ba to 934e9d4 Compare March 19, 2025 21:08
@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch 6 times, most recently from d5f2ca0 to b2c11b2 Compare March 31, 2025 05:41
@rghaddab rghaddab marked this pull request as ready for review March 31, 2025 05:41
@rghaddab rghaddab requested review from LuDuda and omkar3141 March 31, 2025 05:42
@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch 2 times, most recently from 8aadee1 to 3022716 Compare March 31, 2025 08:03
@rghaddab rghaddab requested a review from de-nordic March 31, 2025 08:05
@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch from 3022716 to 9fec112 Compare March 31, 2025 08:08
@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch 2 times, most recently from 67900bf to 0b14b25 Compare April 2, 2025 11:52
@rghaddab rghaddab added this to the ncs-3.0.0 milestone Apr 2, 2025
@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch 3 times, most recently from e166a69 to 3a0a404 Compare April 3, 2025 12:04
Copy link
Contributor

@omkar3141 omkar3141 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check sonar cloud issue and see if you can resolve as many as possible if implementation is settled?

I haven't re-read the full PR yet.

/* Default callback to get the value's length of the Key defined by name. Returns 0 if Key
* doesn't exist.
*/
static int settings_get_val_len_default_cb(const char *name, size_t len, settings_read_cb read_cb,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like read_cb and cb_arg is unused here.

@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch from 3a0a404 to 329de8f Compare April 4, 2025 11:07
rghaddab and others added 18 commits April 11, 2025 16:27
To avoid collisions between IDs used by settings and IDs used directly
by subsystems using ZMS API, the MSB is always set to 1 for Setting's
name ID written to ZMS backend

Add as well a recovery path if the hash linked list is broken.

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 333fadd)
Add reference to the new settings backend ZMS and add more details about
how it works.

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit f363015)
- Allows to build and run ZMS tests on real platforms.
- Enables ZMS tests designed for a flash-simulator only when
  built for qemu_x86 or native_sim targets.

Signed-off-by: Andrej Butok <[email protected]>
(cherry picked from commit b826d46)
- Sets CONFIG_LOG_BLOCK_IN_THREAD for the zms sample.
- Fixes "--- 9999 messages dropped ---".

Signed-off-by: Andrej Butok <[email protected]>
(cherry picked from commit ac7790e)
When deleting a settings entry the linked list is updated by removing
the deleted node. This operation is time consuming and add some delays.
For applications that use almost the same set of Setting entries, add an
option to make this operation faster at a cost of having some empty
nodes in the linked list.
These empty nodes can be used later when the deleted entry is written
again.
Each empty node occupies 16B of storage.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 02ab54e077e090e4c9364e4d4954a8c3508e513e)
When the CONFIG_ZMS_NO_DOUBLE_WRITE is not enabled there is no need to
search in the cache for matching ID

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 546ed2dbe817794bedb16bae0c81fd4af22675ea)
Settings subsystem is storing the name ID and the linked list node ID
with only one bit difference at BIT(0).
Settings subsystem is also storing the name ID and the data ID in two
different ZMS entries at an exact offset of ZMS_DATA_ID_OFFSET.
Using the regular lookup function could result in many cache misses.

Therefore, to assure the least number of collisions in the lookup cache,
the BIT(0) of the hash indicates whether the given ZMS ID represents a
linked list entry or not, the BIT(1) indicates whether the ZMS ID is a
name or data and the remaining bits of the hash are set to a truncated
part of the original hash generated by Settings.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit ad2581fede5f6efa570e3e8157ff37290d2dbe21)
If the subtree argument is not NULL in the settings_load function, load
only the setting entry that corresponds to that subtree.
If the subtree argument is NULL or it is not found in the storage, load
all the existing entries in the storage.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 86132feb5c59e8a071d9782c5482ab346cfbf6f3)
Increase the load performance by adding an optional cache for the linked
list hashes.
This is used only when the settings_load is called with NULL parameter
and we need to load all Settings that exist in the persistent storage.
Cache is enabled using SETTINGS_ZMS_LL_CACHE and the size of the cache
is set using SETTINGS_ZMS_LL_CACHE_SIZE.
Each cache entry will add 8 bytes of RAM usage.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 50c099243557a49b53d793c839299e0a55370f42)
…s entry

Add a a function settings_load_one that allows to load only one settings
entry instead of a complete subtree.
Add as well its implementation for ZMS backend.
This will allow a faster return if the Settings entry doesn't exist in
the persistent storage.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 98e544b4b838cd96012bb4bd5adfb584e67a52f1)
…ad_one

Add a test for the new API settings_load_one that loads only one path
from the persistent storage.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 168ae245bdce756fd5db22dd391469ad17eeb9b2)
Add the functional test for the new backend of ZMS

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 18f7736ce3c73f6f10e037aa627293104dc55b9d)
When a power off happens after writing the settings name and before
writing the linked list node we cannot write the settings name again in
the future.
Fix this by writing the linked list node before writing the name.
When loading all settings, we already delete linked list node that do
not have any name or value written.
Adds as well a recover path if a power down happens in the middle of
unlinking an LL node after a delete.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit bf4d15316b1a3bd9239ddbe179b11006eea1e3b1)
When the linked list is broken, recover it instead of reinitializing it.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 057c1d65fa5822d9145330c87183d0b652707ab2)
Add a function to get the value's length of a Key. If it doesn't exist
returns 0.
Add ZMS implementation for csi_get_val_len() and a default implementation
for the other storage systems.
Add some functional tests to verify it.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 5a730f71f49e87fb4da35120826e8d45bc62fef6)
…f strlen

if the provided name in argument is not null this could lead to un
undefined behavior.
Use strnlen to make this safe

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit e9ac2ef95bd34a8c4756e46e8f2ddf66e0964dbc)
Add references to the new API functions that were added
"csi_load_one" and "csi_get_val_len"

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit b26ed3a3ec5dae5017ecd4e40164e3b38ca88235)
Clean some parts of the code and refactor it to avoid multiple nested
conditions.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>
(cherry picked from commit 1699785ff3fd7d7e260133898729afff634eea73)
@rghaddab rghaddab force-pushed the rghaddab/settings-zms-optimizations branch from 0992192 to a08cf11 Compare April 11, 2025 14:28
@sonarqubecloud
Copy link

@carlescufi
Copy link
Contributor

@omkar3141 @Damian-Nordic please take another look

@carlescufi carlescufi dismissed omkar3141’s stale review April 12, 2025 11:37

stale, sonarcloud issues fixed

@carlescufi carlescufi merged commit 749f6b5 into nrfconnect:main Apr 12, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport v4.0.99-ncs1-branch Relates to NCS v3.0-branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants